Add more properties controlling service reference code generation #10641

dougbu · 2019-05-30T05:14:24Z

$(OpenApiGenerateBeforeCompile) controls if targets run before compile targets
- Make it possible to change when client code generation runs #4924
- also correct multiple invocations when project has multiple target frameworks
$(OpenApiBuildReferencedProjects) controls whether @(OpenApiProjectReference) items build automatically
- Add mechanism to disable project build for ProjectServiceReference #6582

also:

add symbols for Microsoft.Extensions.ApiDescription.Client task assembly
- Missing symbols for the Microsoft.Extensions.ApiDescription.Client package #10508
unconditionally run OpenApiGetDocuments target in referenced projects
- corrects compilation in design-time builds
- no longer uses @(ProjectReferenceWithConfiguration); referenced project chooses all property values

nits:

don't remove $(TargetFrameworks) when invoking OpenApiGetDocuments target (not necessary)
consolidate into a single $(GenerateOpenApiReferenceCodeDependsOn) property
rename task assembly and namespaces in Microsoft.Extensions.ApiDesription.Client to match the project
allow OpenApiGetDocuments targets to run in parallel if $(BuildInParallel) is enabled
remove $(OpenApiDefaultOutputDirectory) normalization; never concatenated with anything else
remove dangling GenerateServiceFileReferenceCode names

dougbu · 2019-05-30T05:24:39Z

...xtensions.ApiDescription.Client/src/build/Microsoft.Extensions.ApiDescription.Client.targets

        RebaseOutputs="true"
-        RemoveProperties="%(ProjectReferenceWithConfiguration.GlobalPropertiesToRemove);TargetFrameworks;RuntimeIdentifier">
+        RemoveProperties="Configuration;Platform;RuntimeIdentifier;TargetFramework;TargetFrameworks">


@rainersigwald is removing these properties sufficient?

And, am I missing something important in the shift from @(ProjectReferenceWithConfiguration) to using @(OpenApiProjectReference) items directly? (Without this, design-time builds would still have been broken and '$(OpenApiBuildReferencedProjects)' == 'false' would not work.)

I'd definitely want to keep something like %(OpenApiProjectReference.GlobalPropertiesToRemove) in there as an escape hatch. This list looks ok to me, but there's always a risk of things changing (like when multitargeting was added) or user projects having their own custom dimensions.

Great suggestion! I added this to #4923 because I have other bits to do short-term.

dougbu · 2019-05-30T05:27:21Z

@natemcmaster I'd of course appreciate any input you have on the MSBuild changes

dougbu · 2019-05-30T22:28:06Z

.../Extensions.ApiDescription.Client/src/build/Microsoft.Extensions.ApiDescription.Client.props

-  <UsingTask TaskName="GetCurrentItems" AssemblyFile="$(_ApiDescriptionTasksAssemblyPath)" />
-  <UsingTask TaskName="GetFileReferenceMetadata" AssemblyFile="$(_ApiDescriptionTasksAssemblyPath)" />
+  <UsingTask TaskName="GetCurrentItems" AssemblyFile="$(_ApiDescriptionClientAssemblyPath)" />
+  <UsingTask TaskName="GetFileReferenceMetadata" AssemblyFile="$(_ApiDescriptionClientAssemblyPath)" />

  <!--
    Settings users may update as they see fit.
  -->
  <PropertyGroup>


@glennc do you have any input on the names of these user-visible properties?

pranavkm

Looks good to me. I wouldn't block on any of my comments.

pranavkm · 2019-05-31T00:17:59Z

...xtensions.ApiDescription.Client/src/build/Microsoft.Extensions.ApiDescription.Client.targets

-  <Target Name="_CreateOpenApiReferenceItemsForOpenApiProjectReferences"
-      AfterTargets="ResolveProjectReferences"
-      Condition="'$(DesignTimeBuild)' != 'true' AND '$(BuildingProject)' == 'true'">
+  <Target Name="_CreateOpenApiReferenceItemsForOpenApiProjectReferences" DependsOnTargets="ResolveProjectReferences">


Should this have Condition="@(OpenApiProjectReference->Count()) != 0"?

It really doesn't matter because everything in this target no-ops when there's nothing worth doing. Let me know if you feel differently

Simple is better, I agree with Doug.

.../Extensions.ApiDescription.Client/src/build/Microsoft.Extensions.ApiDescription.Client.props

pranavkm · 2019-05-31T00:25:15Z

...escription.Client/src/buildMultiTargeting/Microsoft.Extensions.ApiDescription.Client.targets

  </Target>
+
+  <Target Name="_TieInGenerateOpenApiReferenceCode"
+      BeforeTargets="BeforeCompile"


Should this be CoreCompile. Would that allow users to do user things ™️ in BeforeCompile?

CoreCompile is late because it comes after the MSBuild targets cache the list of references and source files involved in the compilation

rynowak · 2019-05-31T20:57:58Z

.../Extensions.ApiDescription.Client/src/build/Microsoft.Extensions.ApiDescription.Client.props

+        Condition="'$(MSBuildRuntimeType)' == 'Core'">netstandard2.0</_ApiDescriptionClientAssemblyTarget>
+    <_ApiDescriptionClientAssemblyTarget
+        Condition="'$(MSBuildRuntimeType)' != 'Core'">net461</_ApiDescriptionClientAssemblyTarget>
+    <_ApiDescriptionClientAssemblyPath>$(MSBuildThisFileDirectory)/../tasks/$(_ApiDescriptionClientAssemblyTarget)/Microsoft.Extensions.ApiDescription.Client.dll</_ApiDescriptionClientAssemblyPath>


Is this really relevant anymore? MSBuild supports netstandard2.0 everywhere .NET Core runs.

If the assembly has any dependencies that aren't in netstandard as implemented in .NET 4.7.2, I'd leave this in. Netstandard task assemblies are probably much more viable now that VS is on 4.7.2, but they've historically been a problem and splitting into core and full implementations has been required.

I'm mentioning it because we've been doing ns2.0 in Razor for a while now with no problems.

Since this is new code it should be fine as long as we test it in VS right?

Yeah, as long as it works in VS, command line msbuild.exe, and command line msbuild.exe from a Build Tools install of VS, you should be good to go.

I've added this to #4923 since the extra bits do no harm. This is all an implementation detail and it'll take time to do the necessary testing…

rynowak · 2019-05-31T21:00:05Z

eng/ProjectReferences.props

@@ -8,7 +8,7 @@
    <ProjectReferenceProvider Include="Microsoft.AspNetCore.Identity.Specification.Tests" ProjectPath="$(RepoRoot)src\Identity\Specification.Tests\src\Microsoft.AspNetCore.Identity.Specification.Tests.csproj"  />
    <ProjectReferenceProvider Include="Microsoft.Web.Xdt.Extensions" ProjectPath="$(RepoRoot)src\SiteExtensions\Microsoft.Web.Xdt.Extensions\src\Microsoft.Web.Xdt.Extensions.csproj"  />
    <ProjectReferenceProvider Include="Microsoft.AspNetCore.DeveloperCertificates.XPlat" ProjectPath="$(RepoRoot)src\Tools\FirstRunCertGenerator\src\Microsoft.AspNetCore.DeveloperCertificates.XPlat.csproj"  />
-    <ProjectReferenceProvider Include="Microsoft.Extensions.ApiDescription.Tasks" ProjectPath="$(RepoRoot)src\Mvc\Extensions.ApiDescription.Client\src\Microsoft.Extensions.ApiDescription.Client.csproj"  />
+    <ProjectReferenceProvider Include="Microsoft.Extensions.ApiDescription.Client" ProjectPath="$(RepoRoot)src\Mvc\Extensions.ApiDescription.Client\src\Microsoft.Extensions.ApiDescription.Client.csproj"  />


I might be missing some necessary context here, but I don't really get the point of renaming an assembly containing tasks to not be suffixed with .Tasks.

Is the metapoint here that we want to separate the tasks used for client-gen from the tasks used inside server-projects?

That was my original point but I ended up not needing tasks in the server project and decided I preferred using the same name everywhere.

rynowak

I left some minor comments here but nothing blocking.

rynowak · 2019-05-31T21:01:48Z

.../Extensions.ApiDescription.Client/src/build/Microsoft.Extensions.ApiDescription.Client.props

-  <UsingTask TaskName="GetCurrentItems" AssemblyFile="$(_ApiDescriptionTasksAssemblyPath)" />
-  <UsingTask TaskName="GetFileReferenceMetadata" AssemblyFile="$(_ApiDescriptionTasksAssemblyPath)" />
+  <UsingTask TaskName="GetCurrentItems" AssemblyFile="$(_ApiDescriptionClientAssemblyPath)" />
+  <UsingTask TaskName="GetFileReferenceMetadata" AssemblyFile="$(_ApiDescriptionClientAssemblyPath)" />


random observation: GetCurrentItems is a pretty generic name given that item is a common term in MSBuild. We might want to choose more specific names. I don't think GetFileReferenceMetadata is bad.

I added the suggestion to #4923

rynowak · 2019-05-31T21:02:12Z

.../Extensions.ApiDescription.Client/src/build/Microsoft.Extensions.ApiDescription.Client.props

+    <!--
+      Options added to the code generator command line by default. Provides the default %(Options) metadata of
+      @(OpenApiReference) and @(OpenApiProjectReference) items.
+    -->


I'm always really happy to see quality docs in MSBuild 👍

rynowak · 2019-05-31T21:03:42Z

.../Extensions.ApiDescription.Client/src/build/Microsoft.Extensions.ApiDescription.Client.props

+
+    <!--
+      If 'true' (the default), generate code for @(OpenApiReference) and @(OpenApiProjectReference) items during
+      design-time builds. Otherwise, generate code only during a full build.


If you're going with the name OpenApiGenerateCodeOnBuild would this become OpenApiGenerateCodeOnDesignTimeBuild?

Thought about it but the sense is a bit different. @glennc and I decided this one was fine as-is. Could of course change it if users disagree.

rynowak · 2019-05-31T21:06:36Z

...xtensions.ApiDescription.Client/src/build/Microsoft.Extensions.ApiDescription.Client.targets

-  <Target Name="_CreateOpenApiReferenceItemsForOpenApiProjectReferences"
-      AfterTargets="ResolveProjectReferences"
-      Condition="'$(DesignTimeBuild)' != 'true' AND '$(BuildingProject)' == 'true'">
+  <Target Name="_CreateOpenApiReferenceItemsForOpenApiProjectReferences" DependsOnTargets="ResolveProjectReferences">


Simple is better, I agree with Doug.

rynowak · 2019-05-31T21:08:17Z

...xtensions.ApiDescription.Client/src/build/Microsoft.Extensions.ApiDescription.Client.targets

@@ -85,10 +79,9 @@
      Condition="$(OpenApiGenerateAtDesignTime) OR ('$(DesignTimeBuild)' != 'true' AND '$(BuildingProject)' == 'true')"
      Inputs="@(OpenApiReference)"
      Outputs="%(OutputPath)">
-    <MSBuild BuildInParallel="$(BuildInParallel)"
-        Projects="$(MSBuildProjectFullPath)"
+    <MSBuild Projects="$(MSBuildProjectFullPath)"


Is this idea with this call that we only want to call this once even with multi-targeting? If that's the case should we have a comment?

Basically I'm not sure why this is a separate invocation of MSBuild, a comment would help.

Yes, this is about running things once. And, sure, I'll add a comment

dougbu · 2019-06-01T03:16:03Z

🆙📅 to add the comment @rynowak suggested. Also rebased on 'master' but deferred most other suggestions to #4923

- `$(OpenApiGenerateBeforeCompile)` controls if targets run before compile targets - #4924 - also correct multiple invocations when project has multiple target frameworks - `$(OpenApiBuildReferencedProjects)` controls whether `@(OpenApiProjectReference)` items build automatically - #6582 also: - add symbols for Microsoft.Extensions.ApiDescription.Client task assembly - #10508 - unconditionally run `OpenApiGetDocuments` target in referenced projects - corrects compilation in design-time builds - no longer uses `@(ProjectReferenceWithConfiguration)`; referenced project chooses all property values nits: - don't remove `$(TargetFrameworks)` when invoking `OpenApiGetDocuments` target (not necessary) - consolidate into a single `$(GenerateOpenApiReferenceCodeDependsOn)` property - rename task assembly and namespaces in Microsoft.Extensions.ApiDesription.Client to match the project - allow `OpenApiGetDocuments` targets to run in parallel if `$(BuildInParallel)` is enabled - remove `$(OpenApiDefaultOutputDirectory)` normalization; never concatenated with anything else - remove dangling `GenerateServiceFileReferenceCode` names

- make it clear these properties control code generation - use "OnBuild" for consistency with properties elsewhere in ASP.NET Core - remove "Default" from a comple of property names because commaents are pretty clear - rename a few targets for consistency with the new properties - word "Reference" didn't add much

…cription.Client.targets

dougbu requested review from pranavkm, rynowak and natemcmaster May 30, 2019 05:14

dougbu requested a review from a team as a code owner May 30, 2019 05:14

dougbu commented May 30, 2019

View reviewed changes

Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label May 30, 2019

dougbu mentioned this pull request May 30, 2019

Add missing symbols for our packages #10539

Closed

dougbu commented May 30, 2019

View reviewed changes

pranavkm approved these changes May 31, 2019

View reviewed changes

JunTaoLuo approved these changes May 31, 2019

View reviewed changes

rynowak reviewed May 31, 2019

View reviewed changes

rynowak approved these changes May 31, 2019

View reviewed changes

dougbu force-pushed the dougbu/client.fixes.4924.6582.10508 branch from 25778bb to 154deaa Compare May 31, 2019 23:15

dougbu mentioned this pull request Jun 1, 2019

Clean up TODOs and duplicated code in client code generation #4923

Closed

3 tasks

dougbu force-pushed the dougbu/client.fixes.4924.6582.10508 branch from 154deaa to 35c789a Compare June 1, 2019 03:15

dougbu added 4 commits June 2, 2019 20:06

Add comment to client tie-in target

58b1ed4

Correct target name in buildMultiTargeting/Microsoft.Extesions.ApiDes…

f1f5f2f

…cription.Client.targets

dougbu force-pushed the dougbu/client.fixes.4924.6582.10508 branch from 35c789a to f1f5f2f Compare June 3, 2019 03:11

dougbu merged commit 2567233 into master Jun 3, 2019

ghost deleted the dougbu/client.fixes.4924.6582.10508 branch June 3, 2019 05:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more properties controlling service reference code generation #10641

Add more properties controlling service reference code generation #10641

dougbu commented May 30, 2019

dougbu May 30, 2019

rainersigwald May 31, 2019

dougbu Jun 1, 2019

dougbu commented May 30, 2019

dougbu May 30, 2019

pranavkm left a comment

pranavkm May 31, 2019

dougbu May 31, 2019

rynowak May 31, 2019

pranavkm May 31, 2019

dougbu May 31, 2019

rynowak May 31, 2019

rainersigwald May 31, 2019

rynowak May 31, 2019

rainersigwald May 31, 2019

dougbu Jun 1, 2019

rynowak May 31, 2019

dougbu Jun 1, 2019

rynowak left a comment

rynowak May 31, 2019

dougbu Jun 1, 2019

rynowak May 31, 2019

rynowak May 31, 2019

dougbu Jun 1, 2019

rynowak May 31, 2019

rynowak May 31, 2019

dougbu Jun 1, 2019

dougbu commented Jun 1, 2019

Add more properties controlling service reference code generation #10641

Add more properties controlling service reference code generation #10641

Conversation

dougbu commented May 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu commented May 30, 2019

Choose a reason for hiding this comment

pranavkm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu commented Jun 1, 2019